[Add] Serve a Gutenberg site in a real WordPress via Playground (#251) - #261
Closed
juanmaguitar wants to merge 3 commits into
Conversation
juanmaguitar
force-pushed
the
juanmaguitar/gutenberg-playground-serve
branch
from
August 11, 2026 10:57
1797b96 to
3d5cd8b
Compare
juanmaguitar
force-pushed
the
juanmaguitar/gutenberg-playground-serve
branch
from
August 11, 2026 13:49
3d5cd8b to
3e81245
Compare
A Core site is served by mounting its build/ as the WordPress docroot, because that build IS a WordPress install. A Gutenberg checkout is a plugin, not a WordPress, so the same path serves an empty site. Make the serve strategy follow the site's project type. - src/playground-plan.cjs: a pure planPlaygroundLaunch(strategy) that returns the @wp-playground/cli mount/install options — 'docroot' mounts build/ as /wordpress and skips the download (Core, unchanged), 'plugin-mount' lets Playground install a stock WordPress and mounts the checkout as an active plugin under wp-content/plugins/<slug> (Gutenberg). This mirrors the CLI's own --auto-mount handling (a `mount` plus an `activatePlugin` step). - server-runner.js now takes a JSON serve config and builds its runCLI options from the plan, keeping the debug + SMTP constants it adds from its own env. - playground:start resolves the strategy from the site's type and hands the runner the checkout to mount (Gutenberg) or the build dir to serve (Core). Verified the plugin-mount path end to end against a throwaway plugin: Playground downloads a stock WordPress, mounts the plugin, and reports a URL that serves WordPress. Core's docroot path is byte-identical. Known gap: the debug.log tail reads a host-side build/wp-content/debug.log, which a plugin-mounted (VFS) WordPress does not write — so the debug-log panel is empty for a Gutenberg site. It degrades quietly (the tail no-ops on a missing file) rather than failing; a Gutenberg-aware debug path is a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
juanmaguitar
force-pushed
the
juanmaguitar/gutenberg-playground-serve
branch
from
August 12, 2026 05:18
3e81245 to
a9d5fc5
Compare
Three findings from the review of the Playground serve path: - playground:start resolved the project type between the in-flight guard and the assignment that arms it, so two concurrent starts for one site both passed the guard and the second orphaned the first — a live PHP-WASM server unreachable to playground:stop and the before-quit sweep, surviving the app quitting and holding its port. The read moves above the guard, restoring the invariant that nothing awaits between the guard and the assignment. - plugin-mount hands Playground a read-write mount of the source checkout, not a regenerable build/, so WordPress could write through to the contributor's working tree: Plugins -> Delete on the mounted plugin unlinks it, .git and uncommitted work included. DISALLOW_FILE_MODS and DISALLOW_FILE_EDIT are now set for that strategy only. Core keeps WordPress's defaults, since it exposes only build/ and a Core contributor may legitimately install a plugin. - The join between the plan and the CLI was unasserted: both strategies were pinned in isolation, so deleting `...launch` from server-runner.js left every test green while Core served a freshly downloaded WordPress with the contributor's build/ nowhere in it. Pinned from both sides, along with the new guards and the race. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmaguitar
added a commit
that referenced
this pull request
Aug 12, 2026
## Why `v1.0.0-beta.1` shipped on 10 August. Twenty changes have landed on `trunk` since — the setup chain, the decoupled build watch, the failed-apply explanation, the ticket's own facts, the toasts. That is the release candidate for 1.0, so the version moves to `1.0.0-rc.1` before the tag is cut. ## What changes `package.json` and `package-lock.json`, and nothing else. Written by `npm version 1.0.0-rc.1 --no-git-tag-version` rather than by hand, so the lockfile's two copies of the string move with it. `git grep 1.0.0-beta` outside the lockfile returns nothing, so no doc, workflow or script carries the version. `electron-builder` derives the artefact names from `package.json`, so the assets become: - `wordpress-contributor-toolkit-1.0.0-rc.1-mac-arm64.dmg` - `wordpress-contributor-toolkit-1.0.0-rc.1-win-x64.exe` - `wordpress-contributor-toolkit-1.0.0-rc.1-linux-x86_64.AppImage` The `rc.1` shape (not `rc1`) keeps the same form as `beta.1`, so the filenames stay in one series. ## Scope The open Gutenberg stack (#255 → #261 → #264 → #269 → #283) is deliberately **not** in this release candidate. An RC stabilises what is there; a feature of that size belongs in the release after it. ## Review No behaviour changes, so the review standard has nothing to grade beyond the diff itself: two version strings, produced by npm, verified against `git grep`. Lint and unit tests run on this branch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…s-a-contribution-target-not-o' into HEAD
juanmaguitar
marked this pull request as draft
August 21, 2026 08:42
Collaborator
Author
|
Closing along with the rest of the stack — see #255 for the reasoning: trunk has rewritten The branch stays, so the work is retrievable. What ports forward cleanly here: src/playground-plan.cjs and test/playground-plan.test.cjs. |
juanmaguitar
added a commit
that referenced
this pull request
Sep 11, 2026
…395) > Rebased out of the Gutenberg stack (#251) and retargeted at trunk. The rest of that stack — #255, #261, #264, #269 — is closed; this change never depended on it, it only sat on top of it. Continues #283, which could not be retargeted because GitHub locks the base of a stacked PR. ## Why Building a Gutenberg site never finished. The wizard sat on "Run build" forever while the app spawned processes without bound — over 1,300 in a few minutes, until the machine was unusable and the app had to be killed. Nothing was ever written to `build/`. The same checkout builds fine outside the app, so this was never a Gutenberg problem. Core sites never hit it either: their build is Grunt, which does not reach the code path below. Fixes #275. ## What changes Root cause: the `node`/`npm`/`npx` shims this app puts on `PATH` are Electron running under `ELECTRON_RUN_AS_NODE`, and Electron keeps `process.versions.electron` set in that mode. `yargs` reads exactly that to decide where a command's arguments begin — "electron set, `defaultApp` unset" reads as a packaged Electron app whose argv carries no script path — so **every yargs-based tool started through a shim treats its own executable path as the first argument it was given**. For a task runner that extra argument is a command to run: itself, with no arguments. The copy it starts does the same, forever. Each link spawns exactly one child, which is why the process tree is an unbounded chain rather than a fan-out. Argument shifting is the general failure here; the runaway processes are only its loudest form. Other tools reached through the shim have been misreading their arguments quietly. The fix: each shim now `--require`s a small module that hides the Electron version from the process it starts. Two decisions worth naming, because both were arrived at by measurement rather than by reasoning: - **An argument, not `NODE_OPTIONS`.** `win-spawn-patch.js` uses `NODE_OPTIONS` and is untouched — it is right there, because it must reach a process several levels down that we never invoke ourselves. Here we *are* the one invoking the process, and `NODE_OPTIONS` did not survive every chain reliably in testing. An argument cannot fail to be inherited, and it confines the patch to processes that actually go through the shim. - **Only `versions.electron` is hidden, not `versions.chrome`.** Hiding both was the plan; it broke Gutenberg's bundling step outright. Build tooling reads `chrome` to decide what it is compiling *for*, which is a question about the output, not about who is running the compiler. Deliberately not in this PR: `versions.v8` still carries its `-electron` suffix (tools parse it as a version number), and the shim directory is still a predictable path under `os.tmpdir()`. ## How to test this Platforms: **any** for the suite. The manual path below was driven on macOS; Windows is covered by unit tests only — see Risks. **Starting state:** a throwaway package whose only script is `concurrently "npm run a" "npm run b"`, with `concurrently@9`, in a site the app can run a script in. The original Gutenberg path is no longer reachable from trunk — that support lived in the closed stack — and it was never needed to see this: the trigger is the task runner, not Gutenberg. 1. Run the script through the app. 2. While it runs, watch the process count: `ps -A | grep -c concurrently` on macOS/Linux. 3. It finishes in about a second. **What must not have happened:** the process count must stay flat — on the broken code the same package reached ~50 processes in three seconds and never recovered. The run must also *finish*: a run that merely stops spawning but hangs is the earlier, subtler half of this bug. Which test covers it, and yes, I checked it fails on the old code: `tests/unit/ipc-wiring.test.cjs` → *"npm:run-script"* now asserts the shims `ensureNodeShimDir` really wrote carry the preload. Blanking the preload path at all six `main.js` call sites — the exact way this regresses — left the entire suite green before that assertion existed, and now fails it. Under `npm run test:electron`, `tests/unit/electron-node-compat.test.cjs` → *"without the preload the child still looks like Electron"* pins the runtime condition itself. ## Risks and limitations Review outcome: **4 `[fix here]` · 3 `[follow-up]` — all 4 fixed.** - **Windows is unit-tested, not hand-tested.** The generated `.cmd`/`.bat` content is asserted directly (quoting, `set` ordering, `%*` last, backslashes kept), and the review checked it line by line, but no one ran a Gutenberg build on a real Windows machine. Buildkite has a signed artifact for this branch if someone wants to. - **Two preload delivery mechanisms now exist** (`NODE_OPTIONS` in `buildChildEnv` for `win-spawn-patch.js`, `--require` in the shims and in the patch's redirects for `electron-node-compat.js`), with different reach and different quoting rules. Nothing ties them together beyond the comments in `node-shims.cjs` and `win-spawn-patch.js`. Consolidating the choice into one documented place is a follow-up. - **The preload reaches forks and the Windows redirects, not every descendant.** `child_process.fork` inherits `execArgv`, so worker pools are covered, and since the 2026-09-10 rebase the Windows spawn patch re-attaches the `--require` to the `node`/`npm`/`npx` spawns it redirects past the shim. A descendant started with an explicit `spawn(process.execPath, …)`, or a `worker_threads` worker, inherits `ELECTRON_RUN_AS_NODE` and sees `versions.electron` again. No such case is known to be reachable today; `NODE_OPTIONS` would cover them, at the cost of the reliability problem that ruled it out. - The shim directory remains world-readable and predictably named under `os.tmpdir()`. This PR adds one more file to a directory that already holds executable shims, so it extends an existing exposure rather than introducing one — but it is worth closing with `mkdtempSync` for all of them. ## Related Fixes #275. Part of #251, whose remaining PRs are closed pending a redo against current trunk. --- <details> <summary>Design decisions and alternatives considered</summary> **Preferring a real system Node over the shim.** Verified to work — the same Gutenberg build completes in 30s through the app's own spawn path once `node` on `PATH` is a real Node. Rejected because it does nothing for a contributor with no Node installed, which is precisely the case the shims exist for: the app's promise is zero prerequisites. **Neutralising only yargs' branch** (setting `process.defaultApp`, the other half of its condition). Narrower, and it would have fixed the runaway. Rejected because it leaves every other library that asks "am I inside Electron?" answering wrongly, which is the general bug. **`NODE_OPTIONS` for the compat preload.** Implemented first, then abandoned: measured, it did not survive every chain from the app down to a task runner's children, while the same preload passed as an argument did. `win-spawn-patch.js` keeps using it because it has no alternative. **Where the shim content lives.** Moved out of `main.js` into `src/node-shims.cjs` as pure string building, so the property that matters — every shim, on every platform, carries the preload — is a unit test rather than something only a real Windows machine could show. </details> <details> <summary>Review outcome (required — see AGENTS.md)</summary> **4 `[fix here]` · 3 `[follow-up]` — all 4 `[fix here]` fixed.** Run per `.github/instructions/code-review.instructions.md`, with the judgement pass given to a subagent with fresh context. Deterministic layer: lint clean, 889 tests pass on both Node runtimes. Fixed: 1. **Nothing tested the wiring that ships the fix.** The reviewer mutated all six `main.js` call sites to pass no preload path and the suite stayed green on both runtimes — the bug could be fully reintroduced without a single red test. The unit tests covered `node-shims.cjs`'s parameters, not the decision to hand it the path. Now `ipc-wiring` reads the shims from disk. 2. **`nodeCompatPath` was passed to `buildChildEnv`, which does not accept it.** Silently dropped, and it read as though descendants were covered through the environment — the exact misreading that would justify removing a `--require` from a shim later. Argument removed. 3. **Two Electron-only tests returned early instead of skipping**, so on the system Node they reported as passing while asserting nothing. Now `t.skip()`, and the two passes no longer report identical counts. 4. **A failed preload copy was reported with `process.stderr.write`**, which `electron-log` does not hook, so a packaged app recorded nothing on the one path that decides whether builds run away — and the write itself sat outside a `try`. Now goes through the app's logger. Deferred, with reasons: - **The test reimplements yargs' `hideBin` heuristic** rather than importing it, so it pins our model of the dependency rather than the dependency. Verified faithful against `yargs` as vendored today. Importing from a transitive dependency in a test is its own trap; left as is, and the comment says what it models. - **The preload does not reach `worker_threads` or an explicit `spawn(process.execPath, …)`.** No reachable case today; noted under Risks so the next reader does not take "an argument always survives" as covering more than it does. - **The shim directory is a predictable path in `os.tmpdir()`.** Pre-existing for the shims and `win-spawn-patch.js`; fixing it properly means `mkdtempSync` for all of them, which is a change to code this PR does not otherwise touch. </details> <details> <summary>Implementation notes</summary> How the root cause was isolated, since the trail is not obvious from the diff: 1. The process tree was a chain of `bash → Electron → bash → Electron`, every one of them running `concurrently` — 25 copies **with no arguments** alongside a single correct invocation. 2. Instrumenting the task runner's `spawn` showed the original process launching *three* children for two commands: its own path, then the two real ones. 3. That pointed at argument parsing rather than at process management, and from there to `hideBin`'s Electron branch. 4. A throwaway package reproduced it in three seconds with no Gutenberg involved — and only with `concurrently@9`, which still uses that yargs path; `@10` does not, which is why a first attempt to reproduce failed and briefly looked like the trigger was elsewhere. `versions.chrome` is the interesting negative result: hiding it removed no recursion (already gone) and broke the bundling step, and there is now a test whose only job is to stop someone widening the set back. </details> --- **Rebase and re-review, 2026-09-10.** Replayed onto trunk at `74e700a` (past the bundled-Git engine, #420 and #422) with no conflicts. The self-review was re-run against the current standard with a fresh-context subagent: **1 `[fix here]` · 1 `[follow-up]`**. - Fixed, in `65c1f7e`: cross-platform 🔴. The compat preload travelled only inside the shims, but on Windows `win-spawn-patch.js` rewrites `spawn('node', …)` straight to Electron's binary, skipping the shim and its `--require`. A tool reached that way saw `versions.electron` again. The route #275 itself takes (`npm run` → `cmd.exe` → `concurrently.cmd` → `node` on PATH) does go through the shim, so the reported failure was covered; the redirect route was not. `buildChildEnv` now exports `WPTK_NODE_COMPAT_PATH` and the patch prepends the same `--require` to its three redirects. Tests by injection in `win-spawn-patch.test.cjs` and `npm-runner.test.cjs`, both red before the change. - Follow-up: the two preload mechanisms, listed under Risks. Re-verified on that head: lint clean, `npm test` 1273 pass / 2 skipped, `npm run test:electron` 1275 pass / 0 skipped. **Manual pass, macOS, 2026-09-10.** The throwaway package from "How to test this" (`concurrently@9.2.4`, `build` = `concurrently "npm run a" "npm run b"`), run through shims generated by this branch's `node-shims.cjs` against the repo's Electron binary. Old-style shims (no preload): 86 `concurrently` processes after six seconds, still climbing, never finished. This branch's shims: both scripts printed, exit 0, no leftover process. **Earlier rebase note.** Replayed onto trunk (`a0fcbc9`) from the closed stack; `src/main.js` and `ipc-wiring` merged without conflict. One extra commit points the two new tests at the `tests/unit/` layout, since they were written before #377 moved unit tests a directory deeper — nothing they assert changed. Re-verified on trunk: lint clean, `npm test` 1058 pass / 0 fail / 2 skipped (the two Electron-runtime tests). --- **Rebase and validation decision, 2026-09-11.** Replayed onto trunk `dd8bc22` with no conflicts. Lint clean, `npm test` 1260 pass / 2 skipped, `npm run test:electron` 1262 pass / 0 skipped. **The Windows manual pass was dropped on purpose, not forgotten.** Recording the reasoning, because the earlier Risks section says someone should run it. There is no reachable trigger from the current product surface. The app's terminal runs only `build`, `build:dev`, `dev`, `test`, `watch` and `grunt`, and in `wordpress-develop` every one of those is Grunt, which parses arguments with nopt rather than yargs. The Gutenberg build that produced the original report is not reachable from trunk: that support lived in a stack that is now closed. An attempt to stage the failure by hand on Windows, with a throwaway `concurrently@9` script installed into a site, ran into the same wall from the other side: the terminal will not run a script outside that list. So what this PR fixes is real but currently latent, and the evidence available matches that status: - macOS, by hand, on the branch's own shims: old shims reached 86 `concurrently` processes in six seconds and never finished; these shims exit 0 and stay flat. - Windows, by unit test: the generated `.cmd`/`.bat` content is asserted directly (quoting, `set` ordering, `%*` last, backslashes kept), `win-spawn-patch` and `npm-runner` cover the redirect route that re-attaches the preload, and `ipc-wiring` reads the shims `ensureNodeShimDir` actually wrote so the fix cannot be removed from the call sites without a red test. What stays open, plainly: nobody has watched a yargs-based task runner start, misparse and recover under these shims on a real Windows machine. If that path becomes reachable again, returning Gutenberg support being the obvious case, run the pass before trusting this on Windows. This is also why the change is not treated as a release blocker: it removes a hazard rather than repairing a failure a contributor can hit today. **CodeRabbit round, 2026-09-11: 1 `[fix here]` fixed · 1 `[follow-up]` filed · 1 declined.** - Fixed in `50a1216`: a failed copy of the compat preload logged a line and wrote the shims without `--require`, which is the runaway state reached silently. Now `ensureNodeShimDir` forgets the directory and throws, and `runNpmWithEngineRetry` reports it through the same "Failed to start" surface as a spawn failure. The realistic trigger is the file missing from the packaged bundle. Wiring test by injection, red before the change. Note for the two Playground handlers that also call `spawnRunner`: they now reject instead of starting without the preload, which is the right outcome for a broken package. - Follow-up #445: the POSIX shims interpolate paths into bash between double quotes, so `$` and backticks in a path expand. Pre-existing in `main.js` before this branch moved the strings; not a security boundary, since whoever controls the app's environment already has `NODE_OPTIONS`. - Declined: running the two Electron-only tests on the system Node by planting a fake `process.versions.electron`. That would assert the fixture, not the runtime; the skips are explicit by design (fix 3 of the first review) and CI runs both passes, so both branches execute. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Part of #251, and the piece that makes a Gutenberg site actually usable: seeing a Gutenberg
change running in a real WordPress. A Core site is served by mounting its
build/as theWordPress docroot, because that build is a WordPress install. A Gutenberg checkout is a plugin,
not a WordPress — mount it as a docroot and you get an empty site. So the serve strategy has to
follow the site's project type.
What changes
src/playground-plan.cjs(new, pure) —planPlaygroundLaunch(strategy)returns the@wp-playground/climount/install options:docroot(Core, unchanged): mountbuild/as/wordpress,wordpressInstallMode: install-from-existing-files-if-needed(skip the download; the build already is WordPress).plugin-mount(Gutenberg): leave the install mode at the default so Playground downloads andinstalls a stock WordPress, then
mountthe checkout under/wordpress/wp-content/plugins/<slug>and add anactivatePluginstep. This mirrors@wp-playground/cli's own--auto-mountplugin handling (verified against the CLI source).server-runner.jsnow takes a JSON serve config (argv[2]) and builds itsrunCLIoptions fromthe plan, still adding the debug + SMTP constants from its own environment. The loopback / hide-
window patches still run before the CLI is required.
playground:startresolves the strategy from the site's project type and hands the runner thecheckout to mount (Gutenberg) or the build dir to serve (Core).
How to test this
Platforms: any. Requires a Gutenberg site (from #255) that has been installed and built.
Automated / mechanism check (no full Gutenberg build needed):
.phpplugin header.node src/server-runner.js '{"strategy":"plugin-mount","pluginDir":"<that dir>","pluginSlug":"smoke"}'.→ It logs
Mount … → /wordpress/wp-content/plugins/smoke, installs a stock WordPress, and printsSERVER_URL:http://127.0.0.1:9400/; opening it serves WordPress. (This is exactly what I ran toverify the plugin-mount path end to end.)
Full flow (a real Gutenberg site):
npm run build).wp-admin → the block editor reflects the built Gutenberg, not core's bundled version.
What must not have happened:
build/as/wordpresswithinstall-from-existing-files-if-needed; the emitted options match the pre-change ones (emptymount/additional-blueprint-stepsare no-ops).test/ipc-wiring.test.cjspins the docrootconfig for a no-type site.
test/runner-wiring.test.cjs), so thedev site is never briefly served to the LAN.
exit(1)s and the start request settleswith an error rather than waiting out the 120s timeout.
What cannot be tested in the suite: the live WASM-PHP boot (network download of stock WP,
minutes on a slow machine). The suite unit-tests the pure planner instead; the boot was verified by
hand as above. Automated:
npm run lintclean, 804 tests pass.Risks and limitations
build/wp-content/debug.log; a plugin-mounted (VFS) WordPress does not write there. It degradesquietly — the watcher no-ops on the missing file/dir, no crash — rather than failing. A
Gutenberg-aware debug path is a follow-up.
pinning a WP version is a future option if a mismatch ever surfaces.
Related
Part of #251. Stacked on #255.
Review outcome (required — see AGENTS.md)
0 [fix here] · 1 [follow-up]. Ran the review in
.github/instructions/code-review.instructions.md; judgement pass in a fresh subagent. Lint clean,804 tests pass.
build/wp-content/debug.log) isCore-shaped; a plugin-mounted Gutenberg WordPress doesn't write there. Verified it degrades
gracefully (watcher no-ops on the missing file/dir), so the panel is inert rather than broken.
Documented above; a Gutenberg-aware debug path rides with the later copy/UX work.
Core docroot option set is behaviorally identical (empty
mount/additional-blueprint-stepsareno-ops,
path.resolvedrop is harmless since the path is already absolute); failure paths areloud (runner
exit(1)settles the start request, no silent hang); untrusted-input handling isfine (JSON built from a registered path,
shell:false); VFS guest paths use POSIX templates whilehost paths use
path.join; tests pin both forks and would fail on a regression.Implementation notes
The plugin-mount recipe is not invented — it's the exact shape
@wp-playground/clibuilds for aplugin passed to
--auto-mount: amountof the host dir to/wordpress/wp-content/plugins/<slug>plus an
{ step: 'activatePlugin', pluginPath }inadditional-blueprint-steps, with the default(download-and-install) WordPress. Keeping the planner pure (
src/playground-plan.cjs) is what letsthe fork be unit-tested without booting WASM PHP;
server-runner.jsmerges the plan with theenv-derived debug/SMTP constants and spawns.